Skip to content

ITEP-163854 - replace react virtuoso - p4 #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 20, 2025

Conversation

camiloHimura
Copy link
Contributor

📝 Description

Add a new hook to scroll a dataset to a specific item index. This will centralize the functionality within MediaItemsList, reducing duplicated code

Dataset List/annotator:

Screen.Recording.2025-05-16.at.10.38.43.mov

Test Dataset List/preview:

Screen.Recording.2025-05-16.at.10.42.56.mov

If referencing an internal ticket (e.g. JIRA), include the ticket number instead:
https://jira.devtools.intel.com/browse/ITEP-32688
-->

✨ Type of Change

Select the type of change your PR introduces:

  • 🐞 Bug fix – Non-breaking change which fixes an issue
  • 🚀 New feature – Non-breaking change which adds functionality
  • 🔨 Refactor – Non-breaking change which refactors the code base
  • 💥 Breaking change – Changes that break existing functionality
  • 📚 Documentation update
  • 🔒 Security update
  • 🧪 Tests

🧪 Testing Scenarios

Describe how the changes were tested and how reviewers can test them too:

  • ✅ Tested manually
  • 🤖 Run automated end-to-end tests

✅ Checklist

Before submitting the PR, ensure the following:

  • 🔍 PR title is clear and descriptive
  • 📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
  • 💬 I have commented my code, especially in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ✅ I have added tests that prove my fix is effective or my feature works

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors scroll logic by introducing a new hook, useScrollToTargetItem, to centralize scrolling behavior in media item list components while also updating view mode settings for layout consistency.

  • Replace react virtuoso scrolling with a custom scroll hook
  • Update view mode settings (e.g., gap change for LARGE view mode)
  • Refactor media and dataset list components (and their tests) to use the new hook

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
web_ui/src/shared/components/media-view-modes/utils.ts Updated view mode settings mapping and layout parameters
web_ui/src/shared/components/media-items-list/use-scroll-to-target-item.hook.tsx Introduced a new hook for scrolling to a target item index
web_ui/src/shared/components/media-items-list/use-scroll-to-target-item.hook.test.tsx Added tests for the new scrolling hook
web_ui/src/shared/components/media-items-list/media-items-list.component.tsx Integrated the new hook into the media items list component
web_ui/src/pages/project-details/components/project-test/test-media-items-list.component.tsx Updated test components to use the new scrollToIndex prop
web_ui/src/pages/project-details/components/project-media/media-content-bucket.component.tsx Applied updated view mode settings for anomaly projects
web_ui/src/pages/annotator/components/sidebar/dataset/dataset-list.component.tsx Refactored scrolling logic to leverage the new hook
web_ui/src/pages/annotator/components/sidebar/dataset/dataset-list-item-details.component.tsx Enhanced media name display with truncation for better layout
Comments suppressed due to low confidence (1)

web_ui/src/shared/components/media-view-modes/utils.ts:15

  • Ensure that the changes in VIEW_MODE_SETTINGS—specifically swapping the positions of LARGE and SMALL and changing the gap for LARGE from 12 to 8—are intentional and aligned with the design requirements.
[ViewModes.LARGE]: { minItemSize: 300, gap: 8, maxColumns: 4 },

@camiloHimura camiloHimura force-pushed the ccolora11/ITEP-163854-replace-react-virtuoso-p4 branch from 1e37f43 to f3bca5d Compare May 19, 2025 06:46
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors scrolling logic by extracting a reusable hook, removes legacy Virtuoso-based scroll code, and updates media list components to use the new hook.

  • Centralize scroll-to-index behavior into useScrollToTargetItem hook
  • Integrate hook into MediaItemsList and replace manual timeouts in higher-level components
  • Remove legacy Virtuoso imports/effects and introduce view mode settings override for anomaly projects

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
shared/components/media-view-modes/utils.ts Reordered view mode settings entries
shared/components/media-items-list/use-scroll-to-target-item.hook.tsx Added useScrollToTargetItem hook with timeout-based scroll
shared/components/media-items-list/use-scroll-to-target-item.hook.test.tsx Added unit tests for the new hook
shared/components/media-items-list/media-items-list.component.tsx Integrated scroll hook, replaced manual scroll logic
pages/project-details/components/project-test/test-media-items-list.component.tsx Removed legacy Virtuoso scroll effect, passed scrollToIndex
pages/project-details/components/project-media/media-content-bucket.component.tsx Added anomaly-specific viewModeSettings, cleaned imports
pages/annotator/components/sidebar/dataset/dataset-list.component.tsx Removed legacy scroll effect and related imports, added scrollToIndex
pages/annotator/components/sidebar/dataset/dataset-list-item-details.component.tsx Updated PressableElement to use isTruncated

@@ -22,7 +22,7 @@ import {
import { MediaDropBoxHeader } from '../../../../shared/components/media-drop/media-drop-box-header.component';
import { MediaDropBox } from '../../../../shared/components/media-drop/media-drop-box.component';
import { MediaItemsList } from '../../../../shared/components/media-items-list/media-items-list.component';
import { INITIAL_VIEW_MODE } from '../../../../shared/components/media-view-modes/utils';
import { INITIAL_VIEW_MODE, VIEW_MODE_SETTINGS, ViewModes } from '../../../../shared/components/media-view-modes/utils';
Copy link
Preview

Copilot AI May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The INITIAL_VIEW_MODE import is unused and can be removed to clean up dead code.

Suggested change
import { INITIAL_VIEW_MODE, VIEW_MODE_SETTINGS, ViewModes } from '../../../../shared/components/media-view-modes/utils';
import { VIEW_MODE_SETTINGS, ViewModes } from '../../../../shared/components/media-view-modes/utils';

Copilot uses AI. Check for mistakes.


const isValidIndex = (index?: number): index is number => !isNil(index) && Number.isInteger(index) && index >= 0;

export const useScrollToTargetItem = ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this name is not really accurate: the hook it self does not scroll, the callback does. Do we have to pass this as a callback parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I intentionally avoided passing the ref as an argument. This hook should focus solely on the calculations and leave the scrolling logic to the handler. Also, the current name isn't very accurate, so I'll update it

container,
targetIndex: scrollToIndex,
dependencies: [scrollToIndex, viewMode, container],
callback: (top) => ref.current?.scrollTo({ top, behavior: 'smooth' }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect react aria to have a specific function to focus on a virtualized element, which then triggers the scrolling. Did you check if there's not a more explicit way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a specific helper or handler in either the react-aria utilities or the component itself

@camiloHimura camiloHimura force-pushed the ccolora11/ITEP-163854-replace-react-virtuoso-p4 branch from f3bca5d to 53ec41b Compare May 19, 2025 09:27
@camiloHimura camiloHimura added this pull request to the merge queue May 20, 2025
Merged via the queue into main with commit b9ee286 May 20, 2025
23 checks passed
@camiloHimura camiloHimura deleted the ccolora11/ITEP-163854-replace-react-virtuoso-p4 branch May 20, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants